feat(rule): stop naming rule rollback on a plan that does not serve it - #435

@claude /review

Claude finished @theCodeDrift's task in 7m 40s —— View job

Review: feat(rule): stop naming rule rollback on a plan that does not serve it

Review mode: incremental — read 0 prior review thread(s) before reviewing. .prior-review.json contained no reviews, review threads, or top-level comments beyond the trigger comment itself, so this is the first review of this PR.

• Read .prior-review.json (data only)
• Gather PR diff (gh pr diff 435) and metadata
• Read the changed source (rules.ts, recover.ts, recovery-advice.ts, identity.ts, org.ts, plan-check.ts) and the full test file
• Cross-check the archived spec deltas against the final openspec/specs/* files for dropped/renamed requirements
• Assess correctness, cleanup, altitude, and CLAUDE.md conventions

Summary

This is a small, well-scoped unit and I did not find any correctness bugs.

• rules.ts's revisionsCommand now threads identity.restoreRules alongside the revisions() result through runForIssuedRule. The await revisions(...) still throws into the same try/catch in runForIssuedRule, so error handling is unchanged. --json output is untouched (revisionsOutputSchema.parse({ success: true, ...list }) never sees restoreRules), matching the spec's "--json output SHALL NOT change" and the new "No extra request is made" scenario — identity.restoreRules rides on the single whoami call already made inside resolveIdentity, no second request is added.
• describeRevisions(list, restoreRules) in recover.ts only changes the closing line on restoreRules === false; the listing itself, the "no revisions" early return, and the "no current revision" line are all unaffected. restore/rollback in the same file don't reference restoreRules at all, consistent with "this is a suggestion, never a gate" — confirmed by the new test still asks the service to restore when whoami says the plan excludes it, which asserts restoreCalled() is true even with restoreRules: false.
• Archive integrity check: I compared the archived delta (openspec/changes/archive/2026-09-30-cli-plan-aware-recovery/specs/*/spec.md) against the final openspec/specs/*/spec.md. All three MODIFIED requirement titles match exactly (Check never writes to the rules tree, Reconcile verdicts are applied per engine, A copy of an issued rule does not run as a local rule, Rule revisions lists what rollback can choose from), and every prior scenario under those requirements is still present verbatim in the final spec plus the new ones — no scenario was silently dropped by the archive, which is the exact failure mode called out in this repo's CLAUDE.md ("A spec delta REPLACES a requirement... Anything you leave out is deleted, silently").
• Test coverage for the new behavior looks adequate: plan-excludes-rollback listing, plan-includes-rollback listing, and the "still calls the service" non-gating case are all covered in rule-recovery.test.ts.

Minor, non-blocking observation

• packages/cli/src/agent/recover-rule.md: the "Rolling back" step-by-step (steps 1–3) doesn't itself repeat the plan check before step 3 ("Roll back to it"); the instruction to check the plan lives only in the "Goal" section above and in "When the plan does not include recovery" near the bottom. In practice step 1's output (rule revisions) would already show the "Rolling back is not included..." line, so an agent following the doc literally still sees the signal before running rollback — this is a documentation-clarity nit, not a functional gap, and I'm not confident it's worth a change.

No other findings. Nice attention to detail on the archive-scenario check called out in the PR description — it held up under a second look.

Re: @claude[bot] — the "Rolling back" steps in agent/recover-rule.md don't repeat the plan check before rule rollback
#435 (comment)

Fixed in 512bd7c, and the gap was larger than the review suggested. Step 1 runs rule revisions --json, and the "Rolling back is not included" line is printed only without --json, so an agent following the recipe never saw it. Step 1 now checks the plan first. It reads check's notices and failures, which --json does carry, or runs rule revisions once without --json. The recipe also says plainly which output carries the signal. --json itself is unchanged, as the spec requires.

— AI Coding Agent